Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add <mstd_xxx> C++ headers #11039

Merged
merged 11 commits into from Jul 19, 2019
Merged

Add <mstd_xxx> C++ headers #11039

merged 11 commits into from Jul 19, 2019

Conversation

kjbracey
Copy link
Contributor

Description

Restructure mbed_cxxsupport.h from #10868, to make it mirror the standard C library.

  • <mstd_xxxx> is equivalent to C++14 or later <xxxx>
  • Gets base toolchain header in to populate namespace std - some local implementation for ARM C 5.
  • Imports all relevant stuff into namespace mstd - alternative implementations supplied where we want to bypass toolchain deficiencies. Eg mstd::mutex maps to rtos::Mutex.
  • namespace mstd can also contain post-C++14 extensions not necessarily present in all toolchains.

Some C++11 library extensions and fixes, notably invoke, <mstd_mutex>.

mbed::Atomic in platform/Atomic.h moved to mstd::atomic in <mstd_atomic> to fit the general pattern.

Using separate namespace and filename gives us the ability to work around toolchain differences. In portable code, the following model could be used, assuming the choice between mbed OS and a fully C++11-compliant system.

#ifdef TARGET_LIKE_MBED
#include <mstd_atomic>
#include <mstd_mutex>
#else
#include <atomic>
#include <mutex>
using namespace mstd = std;
#endif

using mstd::mutex;
 
mutex m;
mstd::atomic_int i;

This is a breaking change with respect to #10868 and #10274 now on master (reworking Atomic.h and mbed_cxxsupport.h), but they're new since 5.13. I'd like this change to go in before 5.14, so there's no visible released API breakage.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@pan-, @bulislaw

Release Notes

  • A set of C++11/14 library support headers have been included in platform/cxxsupport. These include standard-C++-like APIs, eg mstd::atomic in <mstd_atomic>, to improve on implementations provided with toolchains. See platform/cxxsupport/README.md for more information.

@ciarmcom
Copy link
Member

@kjbracey-arm, thank you for your changes.
@bulislaw @pan- @ARMmbed/mbed-os-core @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-test please review.

Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, should be accompanied by good handbook docs to explain how and why to use it.

* A pointer to the singleton, or NULL if not
* initialized.
*/
T *get_no_init() const
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth to add an assert here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not here - I think returning NULL if it's not initialised is potentially useful API. You might want to perform some operation if it's active, but not start it.

Callers could maybe assert, but I don't think asserting ahead of a NULL dereference is that useful either - it will go bang with a pretty easy to debug backtrace.

@@ -160,9 +160,9 @@ def version_check(self):

def _get_toolchain_labels(self):
if getattr(self.target, "default_toolchain", "ARM") == "uARM":
return ["ARM", "ARM_MICRO"]
return ["ARM", "ARM_MICRO", "ARMC5"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Means we can optionally include stuff via a TOOLCHAIN_ARMC5 folder. At the minute we only have the labels TOOLCHAIN_ARM and TOOLCHAIN_ARMC6. Used for the base <array>, <initializer_list> bits for ARM C 5.

(We only need to tiptoe around with namespace mstd and <mstd_xxx> for places where we are up against an existing header file; when we're supplying something ARM C 5 lacks altogether, we can just go straight in with the standard name in namespace std.)

@mbed-ci
Copy link

mbed-ci commented Jul 16, 2019

Test run: FAILED

Summary: 4 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-ARM

@kjbracey kjbracey force-pushed the mstd branch 2 times, most recently from a588881 to ed7909e Compare July 16, 2019 15:15
@kjbracey kjbracey mentioned this pull request Jul 17, 2019
@mbed-ci
Copy link

mbed-ci commented Jul 17, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR

@mbed-ci
Copy link

mbed-ci commented Jul 17, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR

@mbed-ci
Copy link

mbed-ci commented Jul 17, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM

@mbed-ci
Copy link

mbed-ci commented Jul 17, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 6
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR

We have some files that are needed for ARMC5 only.
For all compilers add post-C++14 stuff to namespace mbed:

* invoke, invoke_result, is_invocable, is_invocable_r,
  is_nothrow_invocable, is_nothrow_invocable_r, unwrap_reference,
  unwrap_ref_decay

For ARM C 5, add C++11 bits based on above to namespace std:

* result_of, reference_wrapper, ref, cref, mem_fn
Treat files with no extension as headers, if they are in the cxxsupport
folder.
As something of a cheat, exclude platform/cxxsupport from astyle.

astyle really can't process the sort of template metaprogramming going
on in there; treat it as a "black box" like the toolchains' own
libraries.
@mbed-ci
Copy link

mbed-ci commented Jul 18, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 7
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR

Regularise the C++ support glue, adding `<mstd_type_traits>` etc.

These include the base toolchain file, backfill `namespace std` as much
as possible for ARM C 5, and then arrange to create unified
`namespace mstd`, which can incorporate toolchain bypasses
(eg `mstd::swap` for ARM C 5, or `mstd::atomic`), and include local
implementations of post-ARM C++14 stuff.

All APIs in `namespace mstd` are intended to function as their
`namespace std` equivalent, and their presence there indicates they
are functional on all toolchains, and should be safe to use in
an Mbed OS build (including not unreasonable memory footprint).

See README.md for more info.
@kjbracey kjbracey force-pushed the mstd branch 2 times, most recently from 13b8f67 to 1bc9b70 Compare July 18, 2019 16:57
* Adjust definition to make the default constructor `constexpr`.
  This permits use in classes that want lazy initialization and their
  own `constexpr` constructor, such as `mstd::mutex`.

* Add `get_no_init()` method to allow an explicit optimisation for
  paths that know they won be the first call (such as
  `mstd::mutex::unlock`).

* Add `destroy()` method to permit destruction of the contained object.
  (`SingletonPtr`'s destructor does not call its destructor - a cheat
  to omit destructors of static objects). Needed if using in a class
  that needs proper destruction.
Add add-standard-as-possible version of C++11 <mutex>.

A lot of the stuff in there is generic, but the actual mutex classes and
call_once need to interface with the OS.

For those, they're not available in ARMC5 or IAR; retargetting would be
necessary for ARMC6 and GCC, and I've yet to investigate how whether
that's possible. So for now I'm using local implementations.

Although `Mutex` in principle could support `timed_mutex` and
`recursive_timed_mutex`, we don't have `chrono` for the time parameters,
so hold off for now.

For the generic stuff like mstd::unique_lock, they are aliased to
std::unique_lock where possible.
`mbed::Atomic<T>` becomes `mstd::atomic<T>`, alongside the other
standard C++ library lookalikes.
Standard library implementation should be fine for other toolchains.
Stripped down headers that route through to `std` forms more - eg
no local implementation of `atomic` or `mutex`.
@mbed-ci
Copy link

mbed-ci commented Jul 18, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 8
Build artifacts

Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work; I skimmed through files (it still took me few hours) and this looks solid. Some tests would help for future maintenance.

@@ -28,6 +30,8 @@ using utest::v1::Case;

namespace {

using mstd::atomic;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mbed.h is included above, so there's using namespace std and using mstd:atomic that both brings atomic in the global namespace.

Copy link
Contributor Author

@kjbracey kjbracey Jul 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually pulling it into this unnamed namespace.

I previously claimed that a global-scope using mstd::atomic would override a more general using namespace std for atomic, and I'm sure I'd tested this, but seemed not to work here.

But putting it into an inner namespace like this does work - it searches innermost namespace first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I somehow missed the anonymous namespace declaration. Good to know.

*
* - provides <algorithm>
* - For ARM C 5, standard C++11/14 features:
* - std::min, std::max for std::initializer_list
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not have <mstd_initializer_list> for initialiser list ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good general question - for consistency, it would indeed make sense to have mstd_xxx for all headers we think are good, to "approve" them, and to allow a single using namespace mstd, even if we currently don't need any adaptation, and it would just copy everything over. If you want to fill them in, I wouldn't object, at least for "not-too-bloaty-for-embedded" stuff.

* - std::find_if_not
* - std::equal (2-range forms)
* - std::copy_n, std::move, std::move_backward
* - mstd::min, mstd::max constexpr replacements
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we get constexpr replacement if std::min and std::max are directly imported ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mstd::min and mstd::max. For ARM C 5 those are these replacement implementations, rather than being aliased to std::min like for the other toolchains.

#define MSTD_CONSTEXPR_OBJ_14 constexpr
#else
#define MSTD_CONSTEXPR_FN_14 inline
#define MSTD_CONSTEXPR_OBJ_14 const
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure to agree with that definition; in C++14 a constexpr object can invoke non const member function in a constexpr context. In C++11; constexpr equals const.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constexpr implies const method in a C++11, yes, but I don't think that's an issue here. That would be used as:

MSTD_CONSTEXPR_FN_14 int my_complicated_method() /* const or not */ {
      // some-multi-line thing
     return whatever;
}

Flipping from constexpr in C++14 to inline in C++11 seems right to me - either member or non-member.

Or were you misunderstanding OBJ - that was intended for a constexpr object:

static MSTD_CONSTEXPR_OBJ_14 int npos = -1;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of the following case:

struct Foo { 
    constexpr Foo() { }

    // not a const member function!
    MSTD_CONSTEXPR_FN_14 int get_something() { /* ... */ }
};

static MSTD_CONSTEXPR_OBJ_14 Foo foo;

void foo() { 
   // error call of a non const function on a const object
   auto v = foo.get_something();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me? In C++14, the function is constexpr, not const, and it fails to compile.

In C++11, the function is inline, not const, and it fails to compile.

User code should normally be able to include C++14 headers and get
most expected functionality. (But bear in mind that many C++ library
features like streams and STL containers are quite heavy and may
not be appropriate for small embnedded use).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

embnedded => embedded

#define MSTD_CONSTEXPR_OBJ_14 constexpr
#else
#define MSTD_CONSTEXPR_FN_14 inline
#define MSTD_CONSTEXPR_OBJ_14 const
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above.

// [refwrap]
template <typename T>
class reference_wrapper {
T &FUN(T &x) noexcept { return x; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question, why use FUN as identifier ? __fun or _Fun where not good enough ?

Copy link
Contributor Author

@kjbracey kjbracey Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It came from the revised-in-2017 standard text (https://cplusplus.github.io/LWG/issue2993). We're not actually using it anyway, as that version seems to be too advanced for ARM C 5. I'll remove the dead code.

}

template <typename F>
impl::not_fn_t<F> not_fn_t(F&& f)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean not_fn as the function name ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spot the person rushing to get this in before holidays. (To ensure we don't have a breaking change versus a shipped mbed_cxxsupport.h)

To some extent this is justified for the bulk of the code being for ARMC5 only, and it's not-officially-supported status, but yes, testing clearly needed.

template<
class T,
class D = default_delete<T>
> class unique_ptr : public impl::deleter_store<D>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to see that class getting in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but sadly in use, it increases code size, at least in my tests.

You'd think it was "free", but the compiler doesn't seem to be as good at optimising out shuffling. For example, given:

 if (!ptr) {
     ptr = make_unique<Thing>(whatever);
 }

I've seen ARMC5 at least put in a pointless delete-of-current-pointer-contents on the assignment (but we just tested that the pointer is currently empty), and then a pointless delete of the temporary returned by make_unique (we know we just moved out of it).

Avoiding make_unique helped a bit, but still not as good as doing it by hand.

Maybe other compilers (and their std::unique_ptr) do better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much as I want to believe the C++ guys' claims that C++ is as efficient as C, I keep seeing cases like this where it's just a bit too hard for the compiler to totally eliminate the generic stuff, and it does get bigger. At this embedded scale of tens of kilobytes of ROM and RAM, the impact is real...

Lots of little micro things you wouldn't think of.

Plus the annoyances like what I hit with SingletonPtr here - took me a number of hours to understand the initialization rules enough to comprehend the "mixed zero and constant initialization" problem I was hitting and deal with it to get them back out of the final link.

Was watching this while debugging that one: https://www.youtube.com/watch?v=D7Sd8A6_fYU

Made me a little grumpy - yes, there are misconceptions about C++ for embedded use, but it does also have real issues. And yes, sure, stronger type checking can move more debugging to compilation time, but often that debugging is much harder...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be sweet if our documentations were explaining which C++ features is safe to use and what are the impacts on code size/speed (I'd love to write that). I hope most of the code won't turn into crazy template so debug ability should be okay; too bad that compilers doesn't have attributes to treat constructors as literals. It would be really useful for basic building blocks such as Callback.

Much as I want to believe the C++ guys' claims that C++ is as efficient as C, I keep seeing cases like this where it's just a bit too hard for the compiler to totally eliminate the generic stuff, and it does get bigger. At this embedded scale of tens of kilobytes of ROM and RAM, the impact is real...

I'd be interested to measure the impact on a real world application. Not that I doubt there is one (there will be one as long as there's no destructive moves) but an accurate picture would help us create guidelines to our community of users and internal developers.

};

// [thread.mutex.recursive]
class recursive_mutex : public _Mutex_base {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any plan to add the timed_ versions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need chrono for ARMC5 first. That feels like a separate project.

If licensing permitted just lifting the ARMC6 implementation...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one case where ARMC5 is a significant blocker. It would be easy to bring chrono in to use for the other 3 toolchains, but any attempt to keep ARMC5 building blocks it. Could make all relevant APIs conditional on ifndef __CC_ARM, but don't fancy that either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants